-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[LifetimeSafety] Add support for GSL Pointer types #154009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: users/usx95/08-19-identify_declrefexpr_as_a_use_of_an_origin
Are you sure you want to change the base?
[LifetimeSafety] Add support for GSL Pointer types #154009
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
f5a6015
to
e63ca31
Compare
c3bced1
to
5abecd1
Compare
e63ca31
to
10c8b84
Compare
10c8b84
to
a5fd9a9
Compare
5abecd1
to
c5dc462
Compare
a5fd9a9
to
afa47aa
Compare
9e7697a
to
2f46b55
Compare
3b3b93b
to
440c3fe
Compare
afa47aa
to
bc12cea
Compare
bc12cea
to
fa5312a
Compare
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) ChangesThis PR extends the lifetime safety analysis to handle GSL pointer types (classes marked with Key improvements:
Full diff: https://github.com/llvm/llvm-project/pull/154009.diff 3 Files Affected:
diff --git a/clang/lib/Analysis/LifetimeSafety.cpp b/clang/lib/Analysis/LifetimeSafety.cpp
index 00efa4c5e548f..f9afa3ff6b5d9 100644
--- a/clang/lib/Analysis/LifetimeSafety.cpp
+++ b/clang/lib/Analysis/LifetimeSafety.cpp
@@ -433,6 +433,31 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
void VisitDeclRefExpr(const DeclRefExpr *DRE) { handleUse(DRE); }
+ void VisitCXXConstructExpr(const CXXConstructExpr *CCE) {
+ if (!isGslPointerType(CCE->getType()))
+ return;
+
+ if (CCE->getNumArgs() > 0 && hasOrigin(CCE->getArg(0)->getType()))
+ // This is a propagation.
+ addAssignOriginFact(*CCE, *CCE->getArg(0));
+ else
+ // This could be a new borrow.
+ checkForBorrows(CCE, CCE->getConstructor(),
+ {CCE->getArgs(), CCE->getNumArgs()});
+ }
+
+ void VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
+ if (!isGslPointerType(MCE->getImplicitObjectArgument()->getType()))
+ return;
+ // Specifically for conversion operators, like `std::string_view p = a;`
+ if (isa<CXXConversionDecl>(MCE->getCalleeDecl())) {
+ // The argument is the implicit object itself.
+ checkForBorrows(MCE, MCE->getMethodDecl(),
+ {MCE->getImplicitObjectArgument()});
+ }
+ // Note: A more general VisitCallExpr could also be used here.
+ }
+
void VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *N) {
/// TODO: Handle nullptr expr as a special 'null' loan. Uninitialized
/// pointers can use the same type of loan.
@@ -492,10 +517,27 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *FCE) {
// Check if this is a test point marker. If so, we are done with this
// expression.
- if (VisitTestPoint(FCE))
+ if (handleTestPoint(FCE))
return;
- // Visit as normal otherwise.
- Base::VisitCXXFunctionalCastExpr(FCE);
+ if (isGslPointerType(FCE->getType()))
+ addAssignOriginFact(*FCE, *FCE->getSubExpr());
+ }
+
+ void VisitInitListExpr(const InitListExpr *ILE) {
+ if (!hasOrigin(ILE->getType()))
+ return;
+ // For list initialization with a single element, like `View{...}`, the
+ // origin of the list itself is the origin of its single element.
+ if (ILE->getNumInits() == 1)
+ addAssignOriginFact(*ILE, *ILE->getInit(0));
+ }
+
+ void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *MTE) {
+ if (!hasOrigin(MTE->getType()))
+ return;
+ // A temporary object's origin is the same as the origin of the
+ // expression that initializes it.
+ addAssignOriginFact(*MTE, *MTE->getSubExpr());
}
void handleDestructor(const CFGAutomaticObjDtor &DtorOpt) {
@@ -521,8 +563,75 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
}
private:
+ static bool isGslPointerType(QualType QT) {
+ if (const auto *RD = QT->getAsCXXRecordDecl()) {
+ // We need to check the template definition for specializations.
+ if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
+ return CTSD->getSpecializedTemplate()
+ ->getTemplatedDecl()
+ ->hasAttr<PointerAttr>();
+ return RD->hasAttr<PointerAttr>();
+ }
+ return false;
+ }
+
// Check if a type has an origin.
- bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
+ static bool hasOrigin(QualType QT) {
+ if (QT->isFunctionPointerType())
+ return false;
+ return QT->isPointerOrReferenceType() || isGslPointerType(QT);
+ }
+
+ /// Checks if a call-like expression creates a borrow by passing a local
+ /// value to a reference parameter, creating an IssueFact if it does.
+ void checkForBorrows(const Expr *Call, const FunctionDecl *FD,
+ ArrayRef<const Expr *> Args) {
+ if (!FD)
+ return;
+
+ for (unsigned I = 0; I < Args.size(); ++I) {
+ if (I >= FD->getNumParams())
+ break;
+
+ const ParmVarDecl *Param = FD->getParamDecl(I);
+ const Expr *Arg = Args[I];
+
+ // This is the core condition for a new borrow: a value type (no origin)
+ // is passed to a reference parameter.
+ if (Param->getType()->isReferenceType() && !hasOrigin(Arg->getType())) {
+ if (const Loan *L = createLoanFrom(Arg, Call)) {
+ OriginID OID = FactMgr.getOriginMgr().getOrCreate(*Call);
+ CurrentBlockFacts.push_back(
+ FactMgr.createFact<IssueFact>(L->ID, OID));
+ // For view creation, we assume the first borrow is the significant
+ // one.
+ return;
+ }
+ }
+ }
+ }
+
+ /// Attempts to create a loan by analyzing the source expression of a borrow.
+ /// This method is the single point for creating loans, allowing for future
+ /// expansion to handle temporaries, field members, etc.
+ /// \param SourceExpr The expression representing the object being borrowed
+ /// from.
+ /// \param IssueExpr The expression that triggers the borrow (e.g., a
+ /// constructor call).
+ /// \return The new Loan on success, nullptr on failure.
+ const Loan *createLoanFrom(const Expr *SourceExpr, const Expr *IssueExpr) {
+ // For now, we only handle direct borrows from local variables.
+ // In the future, this can be extended to handle MaterializeTemporaryExpr,
+ // etc.
+ if (const auto *DRE =
+ dyn_cast<DeclRefExpr>(SourceExpr->IgnoreParenImpCasts())) {
+ if (const auto *VD = dyn_cast<ValueDecl>(DRE->getDecl())) {
+ AccessPath Path(VD);
+ return &FactMgr.getLoanMgr().addLoan(Path, IssueExpr);
+ }
+ }
+ return nullptr;
+ }
template <typename Destination, typename Source>
void addAssignOriginFact(const Destination &D, const Source &S) {
@@ -534,7 +643,7 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
/// Checks if the expression is a `void("__lifetime_test_point_...")` cast.
/// If so, creates a `TestPointFact` and returns true.
- bool VisitTestPoint(const CXXFunctionalCastExpr *FCE) {
+ bool handleTestPoint(const CXXFunctionalCastExpr *FCE) {
if (!FCE->getType()->isVoidType())
return false;
@@ -612,10 +721,13 @@ class FactGenerator : public RecursiveASTVisitor<FactGenerator> {
for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
FactGeneratorBlockRAII BlockGenerator(FG, Block);
for (const CFGElement &Element : *Block) {
- if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
+ if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>()) {
+ DEBUG_WITH_TYPE("PrintCFG", llvm::dbgs() << " ================== \n");
+ DEBUG_WITH_TYPE("PrintCFG", CS->dump());
+ DEBUG_WITH_TYPE("PrintCFG", CS->getStmt()->dumpColor());
TraverseStmt(const_cast<Stmt *>(CS->getStmt()));
- else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
- Element.getAs<CFGAutomaticObjDtor>())
+ } else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
+ Element.getAs<CFGAutomaticObjDtor>())
FG.handleDestructor(*DtorOpt);
}
}
diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp
index 660b9c9d5e243..bc8a5f3f7150f 100644
--- a/clang/test/Sema/warn-lifetime-safety.cpp
+++ b/clang/test/Sema/warn-lifetime-safety.cpp
@@ -6,6 +6,12 @@ struct MyObj {
MyObj operator+(MyObj);
};
+struct [[gsl::Pointer()]] View {
+ View(const MyObj&); // Borrows from MyObj
+ View();
+ void use() const;
+};
+
//===----------------------------------------------------------------------===//
// Basic Definite Use-After-Free (-W...permissive)
// These are cases where the pointer is guaranteed to be dangling at the use site.
@@ -20,12 +26,31 @@ void definite_simple_case() {
(void)*p; // expected-note {{later used here}}
}
+void definite_simple_case_gsl() {
+ View v;
+ {
+ MyObj s;
+ v = s; // expected-warning {{object whose reference is captured does not live long enough}}
+ } // expected-note {{destroyed here}}
+ v.use(); // expected-note {{later used here}}
+}
+
void no_use_no_error() {
MyObj* p;
{
MyObj s;
p = &s;
}
+ // 'p' is dangling here, but since it is never used, no warning is issued.
+}
+
+void no_use_no_error_gsl() {
+ View v;
+ {
+ MyObj s;
+ v = s;
+ }
+ // 'v' is dangling here, but since it is never used, no warning is issued.
}
void definite_pointer_chain() {
@@ -39,6 +64,16 @@ void definite_pointer_chain() {
(void)*q; // expected-note {{later used here}}
}
+void definite_propagation_gsl() {
+ View v1, v2;
+ {
+ MyObj s;
+ v1 = s; // expected-warning {{object whose reference is captured does not live long enough}}
+ v2 = v1;
+ } // expected-note {{destroyed here}}
+ v2.use(); // expected-note {{later used here}}
+}
+
void definite_multiple_uses_one_warning() {
MyObj* p;
{
@@ -78,6 +113,19 @@ void definite_single_pointer_multiple_loans(bool cond) {
(void)*p; // expected-note 2 {{later used here}}
}
+void definite_single_pointer_multiple_loans_gsl(bool cond) {
+ View v;
+ if (cond){
+ MyObj s;
+ v = s; // expected-warning {{object whose reference is captured does not live long enough}}
+ } // expected-note {{destroyed here}}
+ else {
+ MyObj t;
+ v = t; // expected-warning {{object whose reference is captured does not live long enough}}
+ } // expected-note {{destroyed here}}
+ v.use(); // expected-note 2 {{later used here}}
+}
+
//===----------------------------------------------------------------------===//
// Potential (Maybe) Use-After-Free (-W...strict)
@@ -94,18 +142,14 @@ void potential_if_branch(bool cond) {
(void)*p; // expected-note {{later used here}}
}
-// If all paths lead to a dangle, it becomes a definite error.
-void potential_becomes_definite(bool cond) {
- MyObj* p;
+void potential_if_branch_gsl(bool cond) {
+ MyObj safe;
+ View v = safe;
if (cond) {
- MyObj temp1;
- p = &temp1; // expected-warning {{does not live long enough}}
- } // expected-note {{destroyed here}}
- else {
- MyObj temp2;
- p = &temp2; // expected-warning {{does not live long enough}}
+ MyObj temp;
+ v = temp; // expected-warning {{object whose reference is captured may not live long enough}}
} // expected-note {{destroyed here}}
- (void)*p; // expected-note 2 {{later used here}}
+ v.use(); // expected-note {{later used here}}
}
void definite_potential_together(bool cond) {
@@ -159,6 +203,16 @@ void potential_for_loop_use_after_loop_body(MyObj safe) {
(void)*p; // expected-note {{later used here}}
}
+void potential_for_loop_gsl() {
+ MyObj safe;
+ View v = safe;
+ for (int i = 0; i < 1; ++i) {
+ MyObj s;
+ v = s; // expected-warning {{object whose reference is captured may not live long enough}}
+ } // expected-note {{destroyed here}}
+ v.use(); // expected-note {{later used here}}
+}
+
void potential_for_loop_use_before_loop_body(MyObj safe) {
MyObj* p = &safe;
for (int i = 0; i < 1; ++i) {
@@ -182,6 +236,19 @@ void potential_loop_with_break(bool cond) {
(void)*p; // expected-note {{later used here}}
}
+void potential_loop_with_break_gsl(bool cond) {
+ MyObj safe;
+ View v = safe;
+ for (int i = 0; i < 10; ++i) {
+ if (cond) {
+ MyObj temp;
+ v = temp; // expected-warning {{object whose reference is captured may not live long enough}}
+ break; // expected-note {{destroyed here}}
+ }
+ }
+ v.use(); // expected-note {{later used here}}
+}
+
void potential_multiple_expiry_of_same_loan(bool cond) {
// Choose the last expiry location for the loan.
MyObj safe;
@@ -258,6 +325,28 @@ void definite_switch(int mode) {
(void)*p; // expected-note 3 {{later used here}}
}
+void definite_switch_gsl(int mode) {
+ View v;
+ switch (mode) {
+ case 1: {
+ MyObj temp1;
+ v = temp1; // expected-warning {{object whose reference is captured does not live long enough}}
+ break; // expected-note {{destroyed here}}
+ }
+ case 2: {
+ MyObj temp2;
+ v = temp2; // expected-warning {{object whose reference is captured does not live long enough}}
+ break; // expected-note {{destroyed here}}
+ }
+ default: {
+ MyObj temp3;
+ v = temp3; // expected-warning {{object whose reference is captured does not live long enough}}
+ break; // expected-note {{destroyed here}}
+ }
+ }
+ v.use(); // expected-note 3 {{later used here}}
+}
+
//===----------------------------------------------------------------------===//
// No-Error Cases
//===----------------------------------------------------------------------===//
@@ -271,3 +360,14 @@ void no_error_if_dangle_then_rescue() {
p = &safe; // p is "rescued" before use.
(void)*p; // This is safe.
}
+
+void no_error_if_dangle_then_rescue_gsl() {
+ MyObj safe;
+ View v;
+ {
+ MyObj temp;
+ v = temp; // 'v' is temporarily dangling.
+ }
+ v = safe; // 'v' is "rescued" before use by reassigning to a valid object.
+ v.use(); // This is safe.
+}
diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
index 13e5832d70050..98862a1a157b8 100644
--- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp
+++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
@@ -11,7 +11,6 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Testing/TestAST.h"
#include "llvm/ADT/StringMap.h"
-#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <optional>
@@ -31,7 +30,13 @@ class LifetimeTestRunner {
LifetimeTestRunner(llvm::StringRef Code) {
std::string FullCode = R"(
#define POINT(name) void("__lifetime_test_point_" #name)
+
struct MyObj { ~MyObj() {} int i; };
+
+ struct [[gsl::Pointer()]] View {
+ View(const MyObj&);
+ View();
+ };
)";
FullCode += Code.str();
@@ -741,5 +746,155 @@ TEST_F(LifetimeAnalysisTest, NoDuplicateLoansForImplicitCastToConst) {
EXPECT_THAT(Helper->getLoansForVar("a"), SizeIs(2));
}
+TEST_F(LifetimeAnalysisTest, GslPointerSimpleLoan) {
+ SetupTest(R"(
+ void target() {
+ MyObj a;
+ View x = a;
+ POINT(p1);
+ }
+ )");
+ EXPECT_THAT(Origin("x"), HasLoansTo({"a"}, "p1"));
+}
+
+TEST_F(LifetimeAnalysisTest, GslPointerConstructFromOwner) {
+ SetupTest(R"(
+ void target() {
+ MyObj al, bl, cl, dl, el, fl;
+ View a = View(al);
+ View b = View{bl};
+ View c = View(View(View(cl)));
+ View d = View{View(View(dl))};
+ View e = View{View{View{el}}};
+ View f = {fl};
+ POINT(p1);
+ }
+ )");
+ EXPECT_THAT(Origin("a"), HasLoansTo({"al"}, "p1"));
+ EXPECT_THAT(Origin("b"), HasLoansTo({"bl"}, "p1"));
+ EXPECT_THAT(Origin("c"), HasLoansTo({"cl"}, "p1"));
+ EXPECT_THAT(Origin("d"), HasLoansTo({"dl"}, "p1"));
+ EXPECT_THAT(Origin("e"), HasLoansTo({"el"}, "p1"));
+ EXPECT_THAT(Origin("f"), HasLoansTo({"fl"}, "p1"));
+}
+
+TEST_F(LifetimeAnalysisTest, GslPointerConstructFromView) {
+ SetupTest(R"(
+ void target() {
+ MyObj a;
+ View x = View(a);
+ View y = View{x};
+ View z = View(View(View(y)));
+ View p = View{View(View(x))};
+ View q = {x};
+ POINT(p1);
+ }
+ )");
+ EXPECT_THAT(Origin("x"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("y"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("z"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("p"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("q"), HasLoansTo({"a"}, "p1"));
+}
+
+// FIXME: Handle loans in ternary operator!
+TEST_F(LifetimeAnalysisTest, GslPointerInConditionalOperator) {
+ SetupTest(R"(
+ void target(bool cond) {
+ MyObj a, b;
+ View v = cond ? a : b;
+ POINT(p1);
+ }
+ )");
+ EXPECT_THAT(Origin("v"), HasLoansTo({}, "p1"));
+}
+
+// FIXME: Handle temporaries.
+TEST_F(LifetimeAnalysisTest, ViewFromTemporary) {
+ SetupTest(R"(
+ MyObj temporary();
+ void target() {
+ View v = temporary();
+ POINT(p1);
+ }
+ )");
+ EXPECT_THAT(Origin("v"), HasLoansTo({}, "p1"));
+}
+
+TEST_F(LifetimeAnalysisTest, GslPointerWithConstAndAuto) {
+ SetupTest(R"(
+ void target() {
+ MyObj a;
+ const View v1 = a;
+ auto v2 = v1;
+ const auto& v3 = v2;
+ POINT(p1);
+ }
+ )");
+ EXPECT_THAT(Origin("v1"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("v2"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("v3"), HasLoansTo({"a"}, "p1"));
+}
+
+TEST_F(LifetimeAnalysisTest, GslPointerPropagation) {
+ SetupTest(R"(
+ void target() {
+ MyObj a;
+ View x = a;
+ POINT(p1);
+
+ View y = x; // Propagation via copy-construction
+ POINT(p2);
+
+ View z;
+ z = x; // Propagation via copy-assignment
+ POINT(p3);
+ }
+ )");
+
+ EXPECT_THAT(Origin("x"), HasLoansTo({"a"}, "p1"));
+ EXPECT_THAT(Origin("y"), HasLoansTo({"a"}, "p2"));
+ EXPECT_THAT(Origin("z"), HasLoansTo({"a"}, "p3"));
+}
+
+TEST_F(LifetimeAnalysisTest, GslPointerLoanExpiration) {
+ SetupTest(R"(
+ void target() {
+ View x;
+ {
+ MyObj a;
+ x = a;
+ POINT(before_expiry);
+ } // `a` is destroyed here.
+ POINT(after_expiry);
+ }
+ )");
+
+ EXPECT_THAT(NoLoans(), AreExpiredAt("before_expiry"));
+ EXPECT_THAT(LoansTo({"a"}), AreExpiredAt("after_expiry"));
+}
+
+TEST_F(LifetimeAnalysisTest, GslPointerReassignment) {
+ SetupTest(R"(
+ void target() {
+ MyObj safe;
+ View v;
+ v = safe;
+ POINT(p1);
+ {
+ MyObj unsafe;
+ v = unsafe;
+ POINT(p2);
+ } // `unsafe` expires here.
+ POINT(p3);
+ }
+ )");
+
+ EXPECT_THAT(Origin("v"), HasLoansTo({"safe"}, "p1"));
+ EXPECT_THAT(Origin("v"), HasLoansTo({"unsafe"}, "p2"));
+ EXPECT_THAT(Origin("v"), HasLoansTo({"unsafe"}, "p3"));
+ EXPECT_THAT(LoansTo({"unsafe"}), AreExpiredAt("p3"));
+}
+
} // anonymous namespace
} // namespace clang::lifetimes::internal
|
if (!isGslPointerType(CCE->getType())) | ||
return; | ||
|
||
if (CCE->getNumArgs() > 0 && hasOrigin(CCE->getArg(0)->getType())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit worried about cases where a view takes an iterator range, chances are good we might not have origins for those iterator types and we would mistakenly think we borrow from these iterators.
/// \return The new Loan on success, nullptr on failure. | ||
const Loan *createLoanFrom(const Expr *SourceExpr, const Expr *IssueExpr) { | ||
// For now, we only handle direct borrows from local variables. | ||
// In the future, this can be extended to handle MaterializeTemporaryExpr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be based on the origins rather than the expressions. Specifically, we might have parts of the expressions in different basic blocks, e.g.: view( cons ? p1 : p2 )
.
This PR extends the lifetime safety analysis to handle GSL pointer types (classes marked with
[[gsl::Pointer()]]
attribute), enabling detection of dangling references in pointer-like types such asstring_view
or other view types.Key improvements: